-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow apply
condition to be a function
#4857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it 😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Since this is a feature to the config interface, we will leave it to the meeting on Friday and asking Evan for input.
Done! By the way, should I update the implementation in #4536 in this PR while we are at it? |
This comment has been minimized.
This comment has been minimized.
I think it is better to do another PR to update the implementation later |
Thanks for the PR @andylizi, it would be great to be able to clean up plugin-legacy 👏🏼 |
Description
apply
can now be a function. Please see #4536 (review) for the original motivation.Additional context
Unfortunately we won't have
ResolvedConfig
ready when we check forapply
and that's not easy to change (we don't want to callconfig
hook on a plugin if it's ultimately not going toapply
, for example). I eventually decided thatUserConfig
andConfigEnv
should probably be enough for most use cases, and those can always fallback toif (...) return
pattern if necessary.It felt a bit awkward trying to write documentation for this (not a native English speaker), any wording suggestions are welcome :D
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).